feat(observability): export logs, metrics, and traces over OTLP - #174
Open
Bnjoroge1 wants to merge 11 commits into
Open
feat(observability): export logs, metrics, and traces over OTLP#174Bnjoroge1 wants to merge 11 commits into
Bnjoroge1 wants to merge 11 commits into
Conversation
Entire-Checkpoint: 01M0GMGVPS35504XE9KQKPGV14
…version Three defects surfaced by reading an exported record. The reason label was wrong. The control plane's `reason` is not a code: the starvation sweep builds a prose sentence that interpolates the job's `runs-on` labels. Passing it through would explode metric cardinality and export workflow content, so the previous code bounded it — but it bounded every value, including the common `reason: None`, to "unknown". That labelled "no reason supplied" and "unrecognized string" identically and made a legitimately failed job unexplainable. Now `None` is `unspecified`, exact codes pass through, and prose is classified on its stable leading phrase, so the starvation sentence becomes `no_runner`. The full message stays on the log record; only the metric dimension is bounded. The event name conflated two records. A terminal `JobStatus` is a status transition, not the separate `JobCompleted` event, but both exported `body: "job.completed"`. The transition is now `job.status.terminal`. `service.version` reported the observability crate's version, which is meaningless to an operator. `ObservabilityConfig::with_service_version` now takes the host binary's version and all three binaries pass it. Tests: five cases for the classifier, including a hostile interpolated `runs-on` and a 1,000-string drive asserting the label set stays at two. Entire-Checkpoint: 01M0GP28K4J52DGX9CMDJVCYF5
Logs were the only signal reaching a backend; metrics were Prometheus-pull only and traces did not exist at all — the HTTP middleware built a span and dropped it. Metrics. `MetricsRegistry::collect` snapshots every instrument as OTLP-ready families, so export scrapes the same instruments `/metrics` renders rather than maintaining a second set. Counters become cumulative monotonic sums, gauges become gauges, and the internal histogram becomes an explicit-bucket histogram with the implicit `+Inf` bucket OTLP requires. Every cumulative point carries the process start as `startTimeUnixNano`, without which a backend reads a restart as a counter reset. Traces. Add real W3C Trace Context: an inbound `traceparent` is adopted so a caller's trace continues through the control plane, a malformed one starts a new root rather than failing the request, and all-zero ids are rejected per the spec. Spans carry the matched route template and finite surface, never the raw URI. Only 5xx sets `Error` — marking 4xx would make every unauthenticated probe look like an outage. Health and metrics probes are suppressed from trace export; they would swamp the store and explain nothing. Log records now carry `traceId`/`spanId` as OTLP fields, not attributes, so a backend can pivot log to trace. One worker drains logs and spans from a shared bounded queue and scrapes metrics on the same tick, so all three share one client, one batching cadence, and one fail-open path. Verified against a pinned single-node OpenObserve: traces, logs and metrics streams all populated; an injected traceparent arrived as trace_id=4bf92f3577b34da6a3ce929d0e0e4736 with a fresh span id; histogram points carry AGGREGATION_TEMPORALITY_CUMULATIVE with bounded attributes (http_route, preloop_surface) and service_version 0.2.0; public probes absent from spans. 24 crate tests including traceparent adoption, malformed rejection, id uniqueness, OTLP shapes, and the +Inf bucket invariant. Entire-Checkpoint: 01M0GPJ7JVYWNJMPJYVE5QW4P6
…ost failures Load testing exposed both halves of this. A sustained run produced 21 `reason="unrecognized"` job completions with no way to find out what they were: the previous change claimed the full message stayed on the log record, but only the bounded code was ever attached, so the prose was unrecoverable. Attach it as `reason.detail`. Logs are not a label space, and without it an `unrecognized` classification is a dead end. With the detail visible the path was obvious — a second never-claimable sentence the classifier did not match: no windows runner is registered with this server, so `runs-on: windows-latest` cannot be scheduled built at runtime_scheduling.rs with the platform interpolated. It is a distinct condition from the starvation sweep and gets its own code rather than folding into `no_runner`: the sweep means "no matching runner appeared within the grace window", which more capacity fixes, while this means the server has no runner of that platform class at all and never will until one is registered. Matching is on the invariant phrase, so any interpolated platform classifies. After the fix a mixed load of 40 workflow submits and 640 reads produced only `no_runner` (56) and `no_platform_runner` (8), with zero `unrecognized`, four route templates and two surfaces. Entire-Checkpoint: 01M0GQDWZHCJVWQ5TQB0XEETTB
…auge
Three review findings, all reproduced live against a running server:
1. Unbounded route labels. `normalize_route` short-circuited on
`path.contains(':')`, returning the raw path as a label value. A colon is
legal inside a path segment, so an unauthenticated 404 like `/evil:1234`
created one permanent series per distinct URI — remote memory exhaustion.
It now matches only exact entries in the template table, and the
parameterized-template branch requires a non-empty child segment so a bare
collection path (`/api/v1/runs`) resolves to its own template instead of
the single-item one.
2. Unbounded method labels. `req.method().to_string()` copied extension
methods (`X-0001`, …) verbatim into the same map. Methods are now
allowlisted to the standard set with an `other` bucket.
3. Leaking active-requests gauge. The gauge was keyed on the full label set
including `status_class`, which the middleware set to a "2xx" placeholder
before the handler ran and overwrote with the real class after — so every
non-2xx request incremented one series and decremented another. The
`2xx` series grew without bound (160 phantom in-flight after a 4xx
storm) and the 4xx/5xx series went negative. The gauge is now keyed on a
status-free `ActiveLabels`, and the decrement runs from a drop guard so
cancellation, panics, and client disconnects on a long poll release the
slot too.
4. Malformed OTLP histograms. Buckets were stored cumulative (Prometheus
`le` semantics) and emitted as-if-disjoint; every observation was counted
once per bucket and the total exceeded `count`. `otlp_bucket_counts` now
differences adjacent cumulative values and uses `count - last` for +Inf,
so the counts conserve the total. Label values are also escaped in the
Prometheus exposition as defense in depth.
Verification: live server after the fix shows 0 phantom active requests, 0
raw evil-route series, methods collapsed to `other`, and every histogram's
+Inf bucket equals its declared count. Tests cover the colon escape, the
collection-route template match, label escaping, gauge increment/decrement
idempotence, and the cumulative-to-disjoint conversion.
Entire-Checkpoint: 01M0GYRVWN21MA39T53T6NB03R
…down flush Three exporter defects from review, all confirmed in code: 1. Signal-specific endpoints were misrouted. A single endpoint was selected with a fallback chain and `/v1/logs`, `/v1/traces`, `/v1/metrics` were appended unconditionally, so `OTEL_EXPORTER_OTLP_TRACES_ENDPOINT= http://collector/v1/traces` produced `/v1/traces/v1/traces` and routed logs and metrics through the trace URL. Resolution is now per signal: the signal-specific variable wins and is used as-is; the generic base gets the suffix. Headers follow the same per-signal pattern with a generic fallback. 2. `partialSuccess` was ignored. A 2xx with rejected records was recorded as full success. The response body is now parsed for rejected counts per signal and a partial rejection is recorded as a failure. 3. No shutdown flush. The runtime's documented "bounded 2s flush" was a comment with no implementation; buffered records were lost on every clean exit. The worker now selects on a shutdown signal, drains all three signal buffers, and the runtime awaits the join inside the 2s bound. Both binaries invoke it on every exit path. 4. Log timestamps were batch-level. Every record in a flush window got the same export-time timestamp, collapsing intra-batch ordering. Each record now carries its enqueue time. 5. The `mark_exited` heartbeat state was dead (nothing called it; a clean Drop deregisters) and `LimitRegistry::register` took the write lock twice; both cleaned up. Env-mutating config tests now serialize on a shared mutex so they cannot race on process-global `OTEL_*` variables. Verified live against OpenObserve: per-signal URLs resolve exactly (signal-specific as-is, generic suffixed), histograms export disjoint bucket counts that conserve `count`, and spans/logs/metrics all flow. Entire-Checkpoint: 01M0GYSD2YVNH359JTEEHD48H6
sample_host is a stub until the cgroup/process sampler lands, so build_fleet_snapshot was emitting cpu_cores: 0.0 and memory_bytes: 0 — a consumer of /api/v1/status could not distinguish an idle fleet from an unmeasured one. VmHostUsage fields are now Option and skipped in JSON when None. Entire-Checkpoint: 01M0GZFAPGDSXX5JP9A2E0T5ZF
TaskSnapshot no longer carries exited — a clean Drop deregisters, so the flag could only ever be false. The stale threshold stays a literal here; it is consolidated into one constant in the follow-up review-fixes PR. Entire-Checkpoint: 01M0GZG5SQ5X8W0GSW9T53DGR2
Jobs carry an enqueue timestamp so the claim path can measure true queue latency instead of a hardcoded placeholder; the field is serde-defaulted so snapshots persisted before the field existed restore as unknown and are skipped. (The claim-path recording lands with the review fixes.) Entire-Checkpoint: 01M0GZHJA9FSX6WRBH43JB9WFF
A known default password on a loopback port is one forwarded-port or one other-local-user away from being public; compose now fails startup until ZO_ROOT_USER_PASSWORD is supplied. Entire-Checkpoint: 01M0GZJ67NNWNJNJ9KEYHPXT78
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| @@ -156,5 +159,8 @@ async fn main() -> anyhow::Result<()> { | |||
| .await?; | |||
There was a problem hiding this comment.
🟡 Medium src/main.rs:159
When serve(...).await? or a ? in the Cert arm returns an error, main exits before observability_runtime.shutdown() runs, dropping buffered telemetry instead of performing the advertised bounded drain. Capture the command result, await shutdown(), and return the captured result afterward.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @crates/preloop-runner-server/src/main.rs around line 159:
When `serve(...).await?` or a `?` in the `Cert` arm returns an error, `main` exits before `observability_runtime.shutdown()` runs, dropping buffered telemetry instead of performing the advertised bounded drain. Capture the command result, await `shutdown()`, and return the captured result afterward.
| // that via `install_fmt_subscriber`. | ||
| Self { | ||
| _handle: handle, | ||
| worker, |
There was a problem hiding this comment.
🟡 Medium src/lib.rs:614
On runner exit, the export worker is detached without draining its queue, so buffered telemetry from the final flush window is lost. ObservabilityRuntime::new stores the JoinHandle, but _observability_runtime is never used to call shutdown() and ObservabilityRuntime has no Drop implementation despite its documented drop behavior; ensure shutdown is awaited before the runner exits.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @crates/preloop-observability/src/lib.rs around line 614:
On runner exit, the export worker is detached without draining its queue, so buffered telemetry from the final flush window is lost. `ObservabilityRuntime::new` stores the `JoinHandle`, but `_observability_runtime` is never used to call `shutdown()` and `ObservabilityRuntime` has no `Drop` implementation despite its documented drop behavior; ensure shutdown is awaited before the runner exits.
Comment on lines
+107
to
+115
| let (version, trace_id, parent_span_id) = (parts[0], parts[1], parts[2]); | ||
| let valid = version.len() == 2 | ||
| && trace_id.len() == 32 | ||
| && parent_span_id.len() == 16 | ||
| && trace_id.chars().all(|c| c.is_ascii_hexdigit()) | ||
| && parent_span_id.chars().all(|c| c.is_ascii_hexdigit()) | ||
| // All-zero ids are explicitly invalid per the spec. | ||
| && trace_id.chars().any(|c| c != '0') | ||
| && parent_span_id.chars().any(|c| c != '0'); |
There was a problem hiding this comment.
🟡 Medium src/export.rs:107
Malformed traceparent headers are adopted instead of starting a new root, allowing invalid trace IDs to corrupt distributed-trace correlation. valid never checks parts[3], accepts forbidden version ff, and is_ascii_hexdigit() admits uppercase IDs; validate the version, flags, and lowercase-hex requirements before adoption.
- let (version, trace_id, parent_span_id) = (parts[0], parts[1], parts[2]);
+ let (version, trace_id, parent_span_id, flags) = (parts[0], parts[1], parts[2], parts[3]);
let valid = version.len() == 2
+ && version != "ff"
+ && version.chars().all(|c| c.is_ascii_hexdigit() && !c.is_ascii_uppercase())
&& trace_id.len() == 32
&& parent_span_id.len() == 16
- && trace_id.chars().all(|c| c.is_ascii_hexdigit())
- && parent_span_id.chars().all(|c| c.is_ascii_hexdigit())
+ && flags.len() == 2
+ && trace_id.chars().all(|c| c.is_ascii_hexdigit() && !c.is_ascii_uppercase())
+ && parent_span_id.chars().all(|c| c.is_ascii_hexdigit() && !c.is_ascii_uppercase())
+ && flags.chars().all(|c| c.is_ascii_hexdigit() && !c.is_ascii_uppercase())🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @crates/preloop-observability/src/export.rs around lines 107-115:
Malformed `traceparent` headers are adopted instead of starting a new root, allowing invalid trace IDs to corrupt distributed-trace correlation. `valid` never checks `parts[3]`, accepts forbidden version `ff`, and `is_ascii_hexdigit()` admits uppercase IDs; validate the version, flags, and lowercase-hex requirements before adoption.
| .ok() | ||
| .filter(|v| !v.trim().is_empty() && v.trim() != "none") | ||
| .map(|v| v.trim_end_matches('/').to_string()) | ||
| .or_else(|| generic.as_ref().map(|g| format!("{g}{suffix}"))) |
There was a problem hiding this comment.
🟡 Medium src/lib.rs:131
Generic endpoints with a query or fragment resolve to the wrong URL: https://collector/base?token=x becomes https://collector/base?token=x/v1/logs, so the request path remains /base instead of /base/v1/logs and exports fail or go to the wrong collector route. Append suffix to the URL path before its query/fragment, or strip those components if they are intentionally unsupported.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @crates/preloop-observability/src/lib.rs around line 131:
Generic endpoints with a query or fragment resolve to the wrong URL: `https://collector/base?token=x` becomes `https://collector/base?token=x/v1/logs`, so the request path remains `/base` instead of `/base/v1/logs` and exports fail or go to the wrong collector route. Append `suffix` to the URL path before its query/fragment, or strip those components if they are intentionally unsupported.
| v.pointer("/partialSuccess/rejectedLogRecords") | ||
| .or_else(|| v.pointer("/partialSuccess/rejectedSpans")) | ||
| .or_else(|| v.pointer("/partialSuccess/rejectedDataPoints")) | ||
| .and_then(|n| n.as_u64()) |
There was a problem hiding this comment.
🟡 Medium src/export.rs:561
rejected_count_from_body returns None for normal OTLP protojson responses such as "rejectedSpans":"1", so post records a partially rejected batch as fully successful. The rejection fields are protobuf int64 values encoded as decimal strings; parse the string representation, while optionally retaining numeric input support.
Suggested change
| .and_then(|n| n.as_u64()) | |
| .and_then(|n| n.as_u64().or_else(|| n.as_str().and_then(|s| s.parse::<u64>().ok()))) |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @crates/preloop-observability/src/export.rs around line 561:
`rejected_count_from_body` returns `None` for normal OTLP protojson responses such as `"rejectedSpans":"1"`, so `post` records a partially rejected batch as fully successful. The rejection fields are protobuf `int64` values encoded as decimal strings; parse the string representation, while optionally retaining numeric input support.
| if k.is_empty() || v.is_empty() { | ||
| None | ||
| } else { | ||
| Some((k.to_string(), v.to_string())) |
There was a problem hiding this comment.
🟡 Medium src/export.rs:618
parse_headers forwards percent-encoded header values unchanged, so Authorization=Basic%20abc reaches the OTLP exporter as Basic%20abc instead of Basic abc, causing authentication and other encoded header values to fail. Percent-decode each parsed value according to the W3C Baggage format before returning it.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @crates/preloop-observability/src/export.rs around line 618:
`parse_headers` forwards percent-encoded header values unchanged, so `Authorization=Basic%20abc` reaches the OTLP exporter as `Basic%20abc` instead of `Basic abc`, causing authentication and other encoded header values to fail. Percent-decode each parsed value according to the W3C Baggage format before returning it.
| cpus: "1.0" | ||
| memory: 2G | ||
| healthcheck: | ||
| test: ["CMD", "sh", "-c", "wget -qO- http://127.0.0.1:5080/healthz || exit 1"] |
There was a problem hiding this comment.
🟡 Medium openobserve/compose.yml:39
The openobserve container is permanently marked unhealthy because its distroless image contains neither sh nor wget, so the healthcheck fails before requesting /healthz. Replace this with an external probe-capable image or an executable present in the pinned image so depends_on: condition: service_healthy can work.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @contrib/openobserve/compose.yml around line 39:
The `openobserve` container is permanently marked unhealthy because its distroless image contains neither `sh` nor `wget`, so the healthcheck fails before requesting `/healthz`. Replace this with an external probe-capable image or an executable present in the pinned image so `depends_on: condition: service_healthy` can work.
| std::env::var(var) | ||
| .ok() | ||
| .filter(|v| !v.trim().is_empty() && v.trim() != "none") | ||
| .map(|v| v.trim_end_matches('/').to_string()) |
There was a problem hiding this comment.
🟡 Medium src/lib.rs:130
Signal-specific endpoints lose their trailing slash, so OTEL_EXPORTER_OTLP_TRACES_ENDPOINT=https://collector/custom/ is resolved to https://collector/custom and requests are sent to a different route than configured. The shared resolve closure applies trim_end_matches('/') to signal-specific values; preserve those values as-is and only normalize the generic base before appending /v1/<signal>.
Suggested change
| .map(|v| v.trim_end_matches('/').to_string()) | |
| .map(|v| v.to_string()) |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @crates/preloop-observability/src/lib.rs around line 130:
Signal-specific endpoints lose their trailing slash, so `OTEL_EXPORTER_OTLP_TRACES_ENDPOINT=https://collector/custom/` is resolved to `https://collector/custom` and requests are sent to a different route than configured. The shared `resolve` closure applies `trim_end_matches('/')` to signal-specific values; preserve those values as-is and only normalize the generic base before appending `/v1/<signal>`.
Comment on lines
+126
to
+132
| let resolve = |var: &str, suffix: &str| { | ||
| std::env::var(var) | ||
| .ok() | ||
| .filter(|v| !v.trim().is_empty() && v.trim() != "none") | ||
| .map(|v| v.trim_end_matches('/').to_string()) | ||
| .or_else(|| generic.as_ref().map(|g| format!("{g}{suffix}"))) | ||
| }; |
There was a problem hiding this comment.
🟡 Medium src/lib.rs:126
A signal-specific "none" value still sends that signal to the generic OTLP endpoint when OTEL_EXPORTER_OTLP_ENDPOINT is set, so operators cannot disable individual signal export. The filter turns "none" into None, but or_else then falls back to generic; handle the explicit "none" case before applying the generic fallback.
- let resolve = |var: &str, suffix: &str| {
- std::env::var(var)
- .ok()
- .filter(|v| !v.trim().is_empty() && v.trim() != "none")
- .map(|v| v.trim_end_matches('/').to_string())
- .or_else(|| generic.as_ref().map(|g| format!("{g}{suffix}")))
- };
+ let resolve = |var: &str, suffix: &str| {
+ match std::env::var(var) {
+ Ok(v) if v.trim() == "none" => None,
+ Ok(v) if !v.trim().is_empty() => {
+ Some(v.trim_end_matches('/').to_string())
+ }
+ _ => generic.as_ref().map(|g| format!("{g}{suffix}")),
+ }
+ };🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @crates/preloop-observability/src/lib.rs around lines 126-132:
A signal-specific `"none"` value still sends that signal to the generic OTLP endpoint when `OTEL_EXPORTER_OTLP_ENDPOINT` is set, so operators cannot disable individual signal export. The filter turns `"none"` into `None`, but `or_else` then falls back to `generic`; handle the explicit `"none"` case before applying the generic fallback.
| start_nanos: u128, | ||
| health: &Arc<ExportHealth>, | ||
| ) { | ||
| flush_logs(client, targets.logs.as_ref(), resource, logs, health).await; |
There was a problem hiding this comment.
🟡 Medium src/export.rs:354
The shutdown path drops every telemetry item still queued in rx, so accepted records are lost when request_shutdown wins the worker’s select!. drain_and_flush only flushes records already moved into logs and spans; drain rx into those buffers before calling this helper (or close and drain the receiver).
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @crates/preloop-observability/src/export.rs around line 354:
The shutdown path drops every telemetry item still queued in `rx`, so accepted records are lost when `request_shutdown` wins the worker’s `select!`. `drain_and_flush` only flushes records already moved into `logs` and `spans`; drain `rx` into those buffers before calling this helper (or close and drain the receiver).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bounded OTLP/HTTP JSON exporter (2048-capacity channel, 5s flush, fail-open, one worker for all three signals) with per-signal endpoint/header resolution — signal-specific URLs used as-is, generic base suffixed —
partialSuccessaccounting, per-record timestamps, and a real 2s shutdown drain. W3C trace context adoption with all-zero-id rejection; cumulative metrics with process-startstartTimeUnixNano; disjoint histogram buckets that conservecount; jobs carry an enqueue timestamp for true queue latency. Pinned single-node OpenObserve reference profile (loopback, required admin password).Part of a stacked series (merge bottom-up):
Summary by cubic
Exports logs, metrics, and traces over OTLP/HTTP JSON via a single bounded background worker. Previously only logs reached a backend, metrics were Prometheus-pull only, and traces were dropped; now all three signals export to configured OTLP endpoints with bounded labels and a graceful shutdown flush.
/v1/{logs|traces|metrics}. The worker batches (capacity 2048, batch 256, 5s flush), fails open, accountspartialSuccess, timestamps each record, and drains for 2s on shutdown.traceId/spanIdfields).+Infbucket; each point includesstartTimeUnixNanofrom process start.otherbucket; the active-requests gauge no longer keys on status; Prometheus exposition escapes label values.job.status.terminal(wasjob.completed); terminationreasonis now a bounded code with full prose onreason.detail(addsno_platform_runneralongsideno_runner).service.versionnow reflects the host binary viawith_service_version; VM host usage fields serialize as absent when unmeasured.Rollout
OTEL_EXPORTER_OTLP_ENDPOINTorOTEL_EXPORTER_OTLP_{LOGS,TRACES,METRICS}_ENDPOINTand optionalOTEL_EXPORTER_OTLP_{…}_HEADERS. Use full URLs for signal-specific variables; the generic base is auto-suffixed.job.completed→job.status.terminal; terminationreasonvalues (now bounded codes); HTTPmethodmay collapse toother; route labels use normalized templates.enqueued_at_unix_nanosto 0 and are skipped for latency until new jobs populate the field.Written for commit 56ca02b. Summary will update on new commits.
Note
Export logs, metrics, and traces over OTLP with background batching worker
ObservabilityConfiginto per-signal endpoints and headers (otel_logs_endpoint,otel_traces_endpoint,otel_metrics_endpoint); a genericOTEL_EXPORTER_OTLP_ENDPOINTgets/v1/<signal>appended as fallbackcollectmethods toHttpMetrics,StoreMetrics,LifecycleMetrics, andMetricsRegistryto snapshot metrics into OTLPMetricFamilystructures with cumulative temporalitytraceparentfor non-public surfaces, export server spans with bounded attributes, and use anActiveGuardthat reliably decrements the active-requests gauge on all exit pathsAppState::emitand stampsQueuedJobwithenqueued_at_unix_nanosfor queue latency measurementHttpMetricsactive gauge is now keyed byActiveLabels(nostatus_class), preventing duplicate Prometheus series for the same printed label set;normalize_routeno longer treats paths containing:as pre-normalized templates, so such paths now map to known templates by prefix rules or fall back to/unknown📊 Macroscope summarized 56ca02b. 13 files reviewed, 24 issues evaluated, 10 issues filtered, 12 comments posted
🗂️ Filtered Issues
crates/preloop-observability/src/lib.rs — 5 comments posted, 11 evaluated, 6 filtered
sanitized_endpointnow inspects onlyotel_logs_endpoint. With a traces-only or metrics-only configuration,Debugreportsotel_endpoint: Noneeven thoughotlp_enabledis true; with different per-signal URLs it reports only the logs destination. This makes the configuration diagnostics incorrect for the newly supported per-signal setup. [ Out of scope (post-validation triage) ]exporterfield represents “any OTLP signal configured,” but downstreamtracing_enabled/export_spantreat its presence as “traces configured.” With a logs-only or metrics-only endpoint, request spans are still generated and enqueued;flush_spansreturns without clearing the buffer whentargets.tracesisNone, so the worker's span vector grows without bound and can eventually OOM the process. Track per-signal enablement or discard buffers for absent targets. [ Cross-file consolidated ]export_log_in_spanenqueues logs whenever any exporter exists, even when only metrics or traces are configured. In that configurationflush_logsreturns immediately for a missing logs target without clearing its buffer, so every emitted log remains in the worker'slogsvector indefinitely; after 256 records every further record also triggers a futile flush. A metrics-only/traces-only deployment that emits logs therefore grows memory without bound and can eventually be OOM-killed. Gate this on the logs target (or make missing-target flushes discard the buffer). [ Cross-file consolidated ]export_log_in_spangates enqueueing only on the sharedExporterexisting, not on a logs target being configured (andexport_span/tracing_enableddo the same for traces). With a metrics-only or traces-only configuration, normal event logs are therefore queued and appended to the worker'slogsbuffer, whileflush_logsimmediately returns when its target isNoneand never clears that buffer. Sustained events grow the vector without bound and can eventually OOM the process; a logs-only configuration similarly accumulates every HTTP span indefinitely. [ Cross-file consolidated ]tracing_enabledreturns true whenever any exporter exists, including logs-only or metrics-only configurations. HTTP middleware therefore enqueues aSpanRecordfor every non-public request even when no traces endpoint exists;flush_spansreturns early without clearing its buffer when the target isNone, so that buffer grows without bound and can eventually exhaust process memory. This should specifically test whether a traces target is configured (or spans without a target must be discarded). [ Out of scope ]shutdownsignals the worker to drain, but the worker's shutdown branch only flushes its already-accumulatedlogs/spansvectors; it never drains records still waiting in thempscreceiver. Thus a clean exit with queued telemetry immediately drops those records, defeating the new shutdown-drain behavior. The shutdown path must consume queuedItems (up to the time/bound policy) before its final flush. [ Out of scope ]crates/preloop-runner-server/src/http_metrics.rs — 0 comments posted, 2 evaluated, 2 filtered
tracedusesObservability::tracing_enabled(), which is true whenever the shared exporter exists, even if only logs or metrics have an endpoint. In that valid per-signal configuration this middleware enqueues a span for every non-public request, whileflush_spansreturns immediately whentargets.tracesisNonewithout clearing its buffer. The worker therefore retains an ever-growingVec<SpanRecord>and can eventually exhaust memory. Trace generation must be gated on a configured traces target (or unsupported-signal buffers must be discarded). [ Cross-file consolidated ]http_metrics_middlewarenow adoptsSpanContext::from_traceparentfor every traced request, but that parser only checks that there are four fields and validates the two IDs; it never validates the trace-flags field and accepts arbitrary version values. Thus malformed headers such as00-<valid trace id>-<valid parent id>-zzare propagated as the caller's trace instead of starting a new root as documented, producing invalid/misassociated telemetry. [ Out of scope ]crates/preloop-runner-server/src/state.rs — 0 comments posted, 1 evaluated, 1 filtered
bounded_termination_reasonchecks the broadcontains("runner is registered with this server")rule before the starvation prefix. Because starvation reasons interpolate user-controlledruns-onlabels, a label containing that phrase makes a real starvation event classify asno_platform_runnerinstead ofno_runner, corrupting the termination metric. Match the stable starvation prefix first or constrain the platform sentence structurally. [ Out of scope (post-validation triage) ]crates/preloop-runner/src/main.rs — 0 comments posted, 1 evaluated, 1 filtered
mainkeepsobservability_runtimein an underscore binding but never callsObservabilityRuntime::shutdown().await. When an OTLP endpoint is configured, normal command completion immediately tears down the Tokio runtime while the export worker may still hold up to a flush window of records, so runner telemetry is lost instead of receiving the implemented bounded drain. The other binaries explicitly invokeshutdown()before returning. [ Out of scope ]